-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8358813: JPasswordField identifies spaces in password via delete shortcuts #25688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Welcome back mickleness! A progress list of the required criteria for merging this PR into |
|
@mickleness This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 40 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@honkar-jdk, @aivanov-jdk, @DamonGuy) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@mickleness The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
|
IMO this PR is a simple response that interfaces with an existing method. I recommend this approach because it should be relatively low-risk, and it is based on existing precedent. By contrast: #25443 does something more aggressive (and interesting). It replaces selectWordAction with selectLineAction. That looks (to me) like a more thorough/better approach to the general problem of accidentally interacting with words in a password field. This PR should resolve any KeyStroke-based actions, but in JDK-8354646 the original complaint had to do with double-clicking the mouse; so this PR wouldn't impact those steps. I haven't explored this thoroughly yet, but glancing through the AccessibleJTextField suggests an AX interface could still have access (via AccessibleActions) to the word-related actions we're trying to restrict access to. If so: this problem (accessing words in a password field) keeps popping up, and removing/replacing the unwanted actions seems like the most thorough response. (Also see JDK-6191897 and JDK-4231444 ). |
test/jdk/javax/swing/JPasswordField/PasswordFieldInputMapWordTest.java
Outdated
Show resolved
Hide resolved
…est.java Co-authored-by: Andrey Turbanov <[email protected]>
Webrevs
|
honkar-jdk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a mismatch between the PR title and JBS title JDK-8358813.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, update the copyright year.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer necessary after merging with master ( see 2bff8e0 )
| for (int condition : new int[] { | ||
| JComponent.WHEN_IN_FOCUSED_WINDOW, | ||
| JComponent.WHEN_FOCUSED, | ||
| JComponent.WHEN_ANCESTOR_OF_FOCUSED_COMPONENT | ||
| }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The array with input map types could be a static constant just above runTest method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is updated.
| if (actionBinding == DefaultEditorKit.deleteNextWordAction || | ||
| actionBinding == DefaultEditorKit.deletePrevWordAction || | ||
| actionBinding == DefaultEditorKit.beginWordAction || | ||
| actionBinding == DefaultEditorKit.endWordAction || | ||
| actionBinding == DefaultEditorKit.selectionBeginWordAction || | ||
| actionBinding == DefaultEditorKit.selectionEndWordAction || | ||
| actionBinding == DefaultEditorKit.previousWordAction || | ||
| actionBinding == DefaultEditorKit.nextWordAction || | ||
| actionBinding == DefaultEditorKit.selectionPreviousWordAction || | ||
| actionBinding == DefaultEditorKit.selectionNextWordAction ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to put the actions in a list or array? Then listOfActions.contains(actionBinding) instead of this long if condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a preference. I pushed an update that wraps them in a TreeSet.
|
Client tests are green. |
This is in response to: openjdk#25688 (comment)
This is in response to: openjdk#25688 (comment)
DamonGuy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with and without the change to AquaKeyBindings. Passes after adding null to the 3 key bindings. Ran on clientlibs tests and all tests still pass. I left a question on the bindings segment for clarification.
test/jdk/javax/swing/JPasswordField/PasswordFieldInputMapWordTest.java
Outdated
Show resolved
Hide resolved
| "shift alt KP_RIGHT", null, | ||
| "alt BACK_SPACE", null, | ||
| "ctrl W", null, | ||
| "alt DELETE", null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change "works" for the test, but I'm not exactly sure what's changed here. When I make my own local test with a JPasswordField, the key binds don't do anything on Aqua. The CTRL+W seems to just input W. ALT+BACK_SPACE seems to just backspace the character before the carat. Can you help me understand this?
Also, I guess you can end in a trailing comma to match the previous format if you want. Either is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DamonGuy it sounds like maybe (?) you're not able to reproduce the original ticket?
Here's a video showing what I experience in JDK 24 vs what I see when I use this PR:
https://go.screenpal.com/watch/cTjfh4nI55H
Does this match what you're observing/experiencing, and if not: what do you see instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DamonGuy I can reproduce the problem with the test, PasswordSelectionWordTest.java, attached to JDK-8358813.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I guess you can end in a trailing comma to match the previous format if you want. Either is OK.
This is a nice suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I added a trailing comma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the video showcase. I was catching up yesterday after being out and I can now see the issue. I can match what is shown now as well. I might have built the image with incomplete changes the first go around.
…est.java Co-authored-by: DamonGuy <[email protected]>
| /** | ||
| * These are all the actions with "word" in their field name. | ||
| */ | ||
| static Collection<String> wordActions = new TreeSet<>(Arrays.asList( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would Set.of do the job?
Perhaps, List.of would be as good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is updated to Set.of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Using Set ensures there are no duplicates in the list.
|
test/jdk/javax/swing/JPasswordField/PasswordFieldInputMapWordTest.java
Outdated
Show resolved
Hide resolved
…est.java Co-authored-by: Alexey Ivanov <[email protected]>
|
/integrate |
|
@mickleness |
|
Client tests are green. I ran a job yesterday with the latest updates. |
|
/sponsor |
|
Going to push as commit 8d73fe9.
Your commit was automatically rebased without conflicts. |
|
@aivanov-jdk @mickleness Pushed as commit 8d73fe9. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
@mickleness You should associate your GitHub account with your OpenJDK id, follow the instructions in Associating your GitHub account and your OpenJDK username. After that, I also recommend following the instructions in the OpenJDK Email section. |
There were a few action bindings available in JPasswordFields in Aqua that let you identify the boundaries of words.
This came to my attention while looking at the related work #25443 . In that PR we said we should iterate across all available L&Fs, so this PR copies that same approach. (The original complaint only focused on Aqua, though.)
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25688/head:pull/25688$ git checkout pull/25688Update a local copy of the PR:
$ git checkout pull/25688$ git pull https://git.openjdk.org/jdk.git pull/25688/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25688View PR using the GUI difftool:
$ git pr show -t 25688Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25688.diff
Using Webrev
Link to Webrev Comment